-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skips authentication only for anonymous auth requests when anonymous-auth is enabled #4097
Skips authentication only for anonymous auth requests when anonymous-auth is enabled #4097
Conversation
…anonymous-auth is enabled Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
src/main/java/org/opensearch/security/auth/BackendRegistry.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
9f10d49
to
afacf33
Compare
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4097 +/- ##
=======================================
Coverage 65.90% 65.91%
=======================================
Files 298 298
Lines 21352 21355 +3
Branches 3475 3477 +2
=======================================
+ Hits 14073 14077 +4
+ Misses 5540 5537 -3
- Partials 1739 1741 +2
|
"Authorization", | ||
"Basic " | ||
+ Base64.getEncoder() | ||
.encodeToString((User.ANONYMOUS.getName() + ":" + randomAsciiAlphanumOfLength(8)).getBytes(StandardCharsets.UTF_8)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-capping conversation on here. Anonymous will mean requests without credentials presented. If a request presents credentials and they are bogus credentials, then it should fail to authenticate the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anonymous auth currently creates an anonymous user in the backend to be added to thread context.
The root problem of this issue is that the current approach creates an anonymous user if no auth credentials were found, which means that the flow would never reach SAML IdP server since this block will return true
authentication is re-requested hence leading to Invalid SAML Configuration
error since it was never able to reach SAML IdP server.
To solve this problem, we need to identify whether the request is coming as anonymous user or not. Following options were considered:
- Add a header
{ _auth_request_type_: anonymous }
to incoming requests from anonymous. But this somehow got overwritten when a request was sent to read dashboards config. So this option was tabled. - Add auth creds from front-end to let backend know that this request is coming as anonymous user. This is the current approach and it allows differentiating auth provider requests with anonymous auth requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not just swap the order of processing by moving the block you linked to after any SAML auth attempts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already tried that. and that is the conversation Craig was referring to, to swap some code around to see if it works. My question is why are we opposed to using dummy creds coming from front-end, when we any way create an anonymous user on the backend? These creds are not requested from an user, instead they are generated on the fly when a user tries to log in as anonymous. In this approach we're not using the password part of the creds, the only thing the backends needs is the username part to match the anonymous user's name.
As I mentioned in the previous comment, both, anonymous auth + any IdP call, expect credentials to be empty in order to follow correct path, and the current implementation skips check over auth domains if anonymous auth is enabled. Once the for loop completes it then enters this else block to assume anonymous user identity.
To avoid adding auth creds, I moved the if block where we assume anonymous user identity at the end post re-requestAuthentication call, but still resulted in 500 for SAML with Invalid SAML config
error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, anonymous auth is considered a sub-part of basic auth (username + password) in the design, so I followed that same pattern in addressing this.
This would only cause some issues for API callers without passing creds. They would now have to pass in a basic auth header with anonymous user name and a random string as password. No experience has changed on front-end.
One possible solution, which is much larger than this bug fix is to re-write the authenticate method to ensure that anonymous auth is always checked at the very end outside loop and other authenticate check, but I'm not sure how would Log in as anonymous
react in the case where multiple auth domains are enabled. How would we identify the type of login request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on the API calls - I have no concerns from front end, since we are hooking up all the buttons anyway and can make the change appropriately, but not sure if it is considered a breaking change to require a username after this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overloading http basic to support anonymous authentication goes against standard practices, this should not be done.
Have you considered a new header/query parameter that indicates the request should be treated as anonymous, such as X-Access-Mode: anonymous
or ?access_as_anonymous=true
as a way for the client to indicate it should not get a 401
or redirected to an IDP for login.
I tried with something similar: The problem at hand is that API users will have a change in behavior i.e. they will now have to pass extra header to specify the auth type, which we do not want and hence we will have to table this PR. In my approach I used auth credentials, i'm only concerned with username here. Also in this approach the anonymous user will not be created in the backend, specifying auth credentials is just a way for front-end to let backend know that this is an anonymous request similar to your suggestion of using a |
@DarshitChanpura take a look at these PRs I made against my local fork that corrects the SAML flow when anonymous is enabled:
SAML actually expects the
when anonymous is enabled, that won't happen since the request succeeds. The PR on my fork adds an action called |
@DarshitChanpura It looks like the issue came from Dashboards around multi-authentication feature, the dashboard's client can be modified so the client's behavior if the 'Anonymous' authentication box is selected in the multi-auth workflow, no? |
This work will be continued with a different approach in: #4152 Closing this PR out. |
Description
This PR fixes a bug where SAML and Anonymous auth cannot co-exist. At present, enabling SAML along with anonymous auth causes SAML Login to fail before it hits the IdP because both SAML and anonymous auth are wired to not have credentials on the first login attempt, causing the authn check against the auth domains to be skipped (see here). This, eventually, results in setting the default user as anonymous (see here) since anonymous auth is enabled.
Security-dashboards companion PR: opensearch-project/security-dashboards-plugin#1817
Issues Resolved
Testing
Check List
- [ ] New functionality has been documentedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.